Thanks for looking at this! This fix reminds me a lot of the fix i tried yesterday: #28361

That is, it seemed like a good approach to me, but @ockham discovered some issues with it on the frontend.

Thanks @ajlende! This is looking good to me; I can confirm that I'm not seeing the glitch I was seeing with #28361, nor the one I was seeing with #28365.

I'll do some more testing...

Looking at the code, I like this fix more than mine. And forgive me for stating the obvious, but we should only merge one of the two 😅

@ockham Thanks for testing, I'm also trying a few more cases. You may come across a visual glitch with repeated backgrounds, and I already have a fix for that one in #28362

Some visual differences on the frontend with Twenty Twenty.

This PR:

#28365:

I also found an issue with left and right alignment

I also found an issue with left and right alignment

Ohh indeed. Twenty Twenty, frontend, right aligned:

(Edit, the horizontal lines are part of 2020's post layout.)

In the editor, for reference:

That last commit seemed to fix the positioning in Twenty Twenty-One

Edit:

Save:

Think I got the height fixed for Twenty Twenty

Edit:

Save:

I think we're getting closer. Editor and frontend are starting to look good in most of the Twenties, with different alignments.

Full width in TT1 still isn't quite working on the frontend:

Editor:

(Edit, pasted wrong screenshot first.)

I think my last change may have fixed that too. Give it a try

I think my last change may have fixed that too. Give it a try

Oh wow, looks like! 🎉

I think I should start creating a test matrix to keep track of the tricky ones:

• Test the Twenties from 2021 backwards.
• Set the Cover blocks' content position.
• Test all possible alignments (left, center, right) (2019-2021: also wide and full width) of the Cover block.
• Always test both editor and frontend.

Known problematic cases:

• 2021, frontend, full width. (#)
• 2020, frontend, right alignment. (#)
• 2014, frontend, center. (#)
• 2011, both editor and frontend, left aligned. (#)

@ajlende Am I missing anything?

My testing hasn't made it back past Twenty Nineteen yet, but for the things that I have tested so far, that's a complete list

I've covered all the Twenties now (although I didn't go through all alignments for each of them), and things are looking good 🎉

It's getting late here, and I'd like to have @jasmussen take a final look at this to weigh in on the CSS changes (and maybe give it some more testing). If all goes well, I'll merge tomorrow (and will release 9.8.1).

I'm sitting here in GMT-10 so I have a lot of daylight left. I'll have something ready for you and Joen in the morning.

Found one bug with the focal point picker in Twenty Twenty (works fine in Twenty Twenty-One), so that might be worth adding to the list of things to check.

edit: It's not a bug with the theme, it appears to be a bug with center alignment applied.

edit2: Now the bug is showing up with other alignments as well. Maybe a JS thing, more investigation is needed.

edit2: Now the bug is showing up with other alignments as well. Maybe a JS thing, more investigation is needed.

I don't know if it is the same thing you're seeing but I just made #28406 to fix an issue I've been seeing with the Focal point picker.

@stokesman That's exactly the issue. I was wondering if it might be related because I moved a piece of code that said, "This explicit height rule is necessary for the focal point picker to work."

https://github.com/WordPress/gutenberg/pull/28404/files#diff-9e449d4b8bd69c9771de8e20c9a1d78798a6934f99e3bfc432df6ceb23def7f4L8

Based on the other CSS I have it doesn't seem like that line is useful, so I might remove it and then test a bunch to confirm.

@stokesman and @ajlende, I added that line:

Based on the other CSS I have it doesn't seem like that line is useful, so I might remove it and then test a bunch to confirm.

I added it when I restored the height: 100%; after the focal point picker had regressed. However the height rule was initially removed from the Cover block to mitigate issues stemming from the Cover being used inside flex based boxes. So if it's possible to remove that rule in a safe way, it would be great. CC: @jffng

Alright, I'm taking a deep dive this morning, and already now I have a few high level thoughts to share:

First off, this PR is good and short. We can potentially make it simpler, I left a comment, but the basics seem correct.

In testing the various twenties, it seems additionally clear that the change from a CSS background to an img element not only was the primary cause of the issue, but likely that there are additional adjacent issues. Specifically, many themes style the img, and that bleeds into the cover now.

Because of those adjacent issues, we might want to strengthen some selectors and add additional rules. But I'm not sure whether that's something for this PR or something best to keep separate.

Specifically, Twenty Eleven is a great theme to test, because it has pretty elaborate rules that target the img directly:

Behold:

A 1px white gray border, padding, and more.

This PR gets closer in addressing that, but it doesn't get all the way:

As far as I'm aware, the only way to prevent this CSS bleed is to use !important, and target every extra selector we encounter. Something like this:

[the crazy cover selector] {
    border: none !important;
    max-width: none !important;
    padding: 0 !important;
    margin: 0 !important;
    width: 100% !important;
    height: 100% !important;
    position: absolute !important;
    top: 0 !important;
    left: 0 !important;
    right: 0 !important;
    bottom: 0 !important;
    object-fit: cover !important;
}

What other things might a theme do? Box-shadow? Outlines? CSS filters? Is there ever an edgecase where we would want to allow a theme's image styles to bleed into the cover, or do we have to treat the img as purely a dumb element?

I'm nervous about going down this nuclear road, but it seems clear from testing that it's an arms race now that the img element is in play. So we kind of have to make a decision where to go.

Alright, I pushed a change to move the height rule back to the style.scss file, and to add a bunch of extra properties to unset, trying to pre-empt what might bleed in from a theme.

That effectively makes this PR identical to #28412, except softer: the rules have high specificity, but not "nuclear" (there are no !importants).

And it solves the TwentyEleven theme case:

I think this is good enough to merge as a fix, and we can always look at whether we need to increase the specificity here.

2020 frontend, right aligned, is back to this:

Prior to 21ad5ef, it was like this:

(Which isn't perfect, but better?)

Let me take a look at that one. Maybe it was the moving of the 100% height, after all.

Yeah, it appears that height: 100%; needs to be in the editor only. Even there, I'd still like to remove it, and #28406 might be our opportunity to do that. But that's for another day.

Before:

After:

Also rebased.

Thanks Joen! ❤️

Okay I'm not sure I rebased that right, but it wasn't strictly necessary, and the branch appears to work as intended still.

Seeing one editor/frontend inconsistency in 2021 (center alignment):

Editor:

Frontend:

Disclaimer, might be pre-existing. My testing is probably overly paranoid now.

Wow that's a good catch!

That's a pre-existing issue, and appears to be a bug with the demo content, no less. Specifically, the demo content has both align center, and align wide applied, which takes on the frontend but not in the editor.

If you delete the cover block from the demo content, and create a fresh one, then align it wide or center in however direction you want to go, it works fine, both in the editor and published.

Wow that's a good catch!

That's a pre-existing issue, and appears to be a bug with the demo content, no less. Specifically, the demo content has both align center, and align wide applied, which takes on the frontend but not in the editor.

If you delete the cover block from the demo content, and create a fresh one, then align it wide or center in however direction you want to go, it works fine, both in the editor and published.

Hmm, interesting. So it seems like the block has the alignwide className set when first loading it:

<!-- wp:cover {"url":"https://cldup.com/Fz-ASbo2s3.jpg","className":"alignwide"} -->
<div class="wp-block-cover has-background-dim alignwide"><img class="wp-block-cover__image-background" alt="" src="https://cldup.com/Fz-ASbo2s3.jpg" data-object-fit="cover"/><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Of Mountains &amp; Printing Presses</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

Then, when I set the alignment to center:

<!-- wp:cover {"url":"https://cldup.com/Fz-ASbo2s3.jpg","align":"center","className":"alignwide"} -->
<div class="wp-block-cover aligncenter has-background-dim alignwide"><img class="wp-block-cover__image-background" alt="" src="https://cldup.com/Fz-ASbo2s3.jpg" data-object-fit="cover"/><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Of Mountains &amp; Printing Presses</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

So now we have "align":"center", but the wide alignment is also kept in className.

Not sure this is a bug with the demo content though. It looks a bit like it's a block migration that's not working properly maybe?

(Anyway, not directly related to this PR. Just noting here FTR and a later fix.)

Yep, there's some weirdness there, but all of that I reproduced in the main branch, so no blockers for this one, I'd say.

Cherry-picked to release/9.8.